Skip to content

ASoC: SOF: fix panic caused by sdev->msg being null #807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
plbossart merged 5 commits into from
Apr 11, 2019
Merged

ASoC: SOF: fix panic caused by sdev->msg being null #807

plbossart merged 5 commits into from
Apr 11, 2019

Conversation

@libinyang
Copy link

@libinyang libinyang commented Apr 10, 2019 •
edited
Loading

In ipc reply interrupt handler, some unexpected ipc reply interrupts happen. In this case, sdev->msg may be NULL pointer. We must handle it by checking the pointer

This patch fixes #767

Signed-off-by: Libin Yang [email protected]

This was referenced Apr 10, 2019
Closed
Closed
@libinyang libinyang changed the title ASoC: SOF: set sdev->msg in a proper place ASoC: SOF: fix panic caused by sdev->msg being null Apr 10, 2019
lyakh
lyakh reviewed Apr 10, 2019
View reviewed changes
sound/soc/sof/intel/hda-ipc.c Outdated
Copy link
Collaborator

@lyakh lyakh Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "spurious" or "unexpected" would be a better description for that interrupt - in the commit message, the comment and the print, in all patches. Also I'd make it a "dev_err" or at least a "dev_warn" because we want to investigate those casses.

lyakh
lyakh requested changes Apr 10, 2019
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline comment

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 10, 2019

Code looks good, but indeed please adjust the language in commit message. I'd vote for "unexpected" as the word. I'd put this in patch title as well, e.g. "handle unexpected IPC reply".

@libinyang
Copy link
Author

libinyang commented Apr 10, 2019

OK. I will modify the patches based on @lyakh @kv2019i

@libinyang
ASoC: SOF: hda-ipc: handle unexpected IPC reply
f6e673b 
Sometimes, driver will receive the unexpected ipc reply from DSP.
The unexpected ipc reply belongs to none ipc sent before in the kernel
driver. In this case, the driver should ignore the unexpected ipc reply.

Signed-off-by: Libin Yang <[email protected]>
@libinyang
ASoC: SOF: hw-spi: handle unexpected IPC reply
0b0d2a7 
Sometimes, driver will receive the unexpected ipc reply from DSP.
The unexpected ipc reply belongs to none ipc sent before in the kernel
driver. In this case, the driver should ignore the unexpected ipc reply.

Signed-off-by: Libin Yang <[email protected]>
@libinyang
ASoC: SOF: bdw: handle unexpected IPC reply
0346760 
Sometimes, driver will receive the unexpected ipc reply from DSP.
The unexpected ipc reply belongs to none ipc sent before in the kernel
driver. In this case, the driver should ignore the unexpected ipc reply.

Signed-off-by: Libin Yang <[email protected]>
@libinyang
ASoC: SOF: byt: handle unexpected IPC reply
91b3bee 
Sometimes, driver will receive the unexpected ipc reply from DSP.
The unexpected ipc reply belongs to none ipc sent before in the kernel
driver. In this case, the driver should ignore the unexpected ipc reply.

Signed-off-by: Libin Yang <[email protected]>
@libinyang
ASoC: SOF: hsw: handle unexpected IPC reply
2f52b3b 
Sometimes, driver will receive the unexpected ipc reply from DSP.
The unexpected ipc reply belongs to none ipc sent before in the kernel
driver. In this case, the driver should ignore the unexpected ipc reply.

Signed-off-by: Libin Yang <[email protected]>
@libinyang libinyang force-pushed the set_msg_of_sdev branch 2 times, most recently from 0215da7 to 2f52b3b Compare April 10, 2019 12:30
plbossart
plbossart requested changes Apr 10, 2019
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@libinyang Can you clarify where sdev->msg is set? It's far from obvious in the code? And the PR description looks borrowed from another PR so it's not clear what you are fixing.
Each PR should really be stand-alone and contain a description of the problem, how you fix it and why your fix is relevant compared to other alternatives.

@libinyang libinyang mentioned this pull request Apr 11, 2019
Closed
@libinyang
Copy link
Author

libinyang commented Apr 11, 2019 •
edited
Loading

@plbossart The sdev->msg is set in sof_ipc_tx_message_unlocked. Yes, I created this new PR because the old PR is just for QA test and it's [DO NOT MERGE]. This PR is the formal one. Please ignore the old one. Sorry for confusion. I have closed that old [DO NOT MERGE] PR and move the discussion in this PR.

@libinyang Can you clarify where sdev->msg is set? It's far from obvious in the code? And the PR description looks borrowed from another PR so it's not clear what you are fixing.
Each PR should really be stand-alone and contain a description of the problem, how you fix it and why your fix is relevant compared to other alternatives.

@libinyang
Copy link
Author

libinyang commented Apr 11, 2019

More early discussion please refer: #798

@plbossart
Copy link
Member

plbossart commented Apr 11, 2019

@libinyang your PR change log shows this:
"The sdev->msg will always be set as sdev->ipc->msg in
sof_ipc_tx_message_unlocked(). Let's set it in snd_sof_ipc_init().
This can avoid the reduplicative setting."

Your code has nothing to do with this change, so I don't know if your PR is in a valid state or not. It's really painful to have to spend time to figure out what you are trying to fix. I am hours away from sending the v6 patches and I have really no time for these shenanigans.

Also I will need @lyakh to approve before I merge any ipc-related change. Thanks.

@plbossart plbossart requested a review from lyakh April 11, 2019 03:37
@libinyang
Copy link
Author

libinyang commented Apr 11, 2019

@plbossart The comments come from my previous version of the patch. The new patch uses another way after discussing with @lyakh . So the comments are out of date.
What should we do in such situation that the updated patch is totally different from the previous one, open a new PR or change the initial comments?

@libinyang your PR change log shows this:
"The sdev->msg will always be set as sdev->ipc->msg in
sof_ipc_tx_message_unlocked(). Let's set it in snd_sof_ipc_init().
This can avoid the reduplicative setting."

Your code has nothing to do with this change, so I don't know if your PR is in a valid state or not. It's really painful to have to spend time to figure out what you are trying to fix. I am hours away from sending the v6 patches and I have really no time for these shenanigans.

Also I will need @lyakh to approve before I merge any ipc-related change. Thanks.

@ranj063
Copy link
Collaborator

ranj063 commented Apr 11, 2019

What should we do in such situation that the updated patch is totally different from the previous one, open a new PR or change the initial comments?

@libinyang please use your judgement in doing the right thing based on the situation. Basically, the ask is that the PR description should be accurate for someone who reads it for the first time to understand and the commits should back up the argument in the description.

@libinyang
Copy link
Author

libinyang commented Apr 11, 2019

@ranj063 OK. Thanks. I will change the comments so we can follow in the same thread.

plbossart
plbossart approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @libinyang, looks good now. Will merge.

@plbossart plbossart merged commit a0de6c8 into thesofproject:topic/sof-dev Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@plbossart plbossart plbossart approved these changes

@lyakh lyakh lyakh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[WHL HDA] firmware boot failure on WHL platform.

5 participants

@libinyang @kv2019i @plbossart @ranj063 @lyakh